Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add version constants for Java 19, 20, 21 #15

Merged
merged 2 commits into from
Aug 31, 2023
Merged

Conversation

@TheMrMilchmann
Copy link

Hate to be that guy but is there any chance to get this merged soon? Ideally alongside support for the added function jboolean IsVirtualThread(JNIEnv *env, jobject obj);. (ref)

@morristai morristai changed the title Add version constants for Java 19 Add version constants for Java 19, 20, 21 Jun 27, 2023
@rib
Copy link
Contributor

rib commented Jun 28, 2023

Yeah, I've recently been slowly trying to look at updating the jni-sys crate a bit.

Ideally I think it would make sense to add the version constants at the same time as adding the new functions, but a follow up PR to add IsVirtualThread for 19 would be good otherwise.

I haven't seen what new API was added for 20 and 21 yet.

@TheMrMilchmann
Copy link

I haven't seen what new API was added for 20 and 21 yet.

Neither of these versions seems to contain user-facing API changes. I believe the new version constants were added because IsVirtualThread was technically only in preview in 19 and 20.

@rib
Copy link
Contributor

rib commented Jun 28, 2023

hmm, that's a pretty unclear concept. I do vaguely recall seeing discussions before about IsVirtualThread being in preview and didn't really know what that was supposed to mean practically.

So is there some kind of change with what can be relied on with the behaviour or IsVirtualThread between versions?

We should be very clear what that notion of being in preview meant here.

Maybe IsVirtualThread should only be exposed once it's out of preview perhaps?

Can it be NULL while it's in preview perhaps?

It would be great if you'd be able to dig up some information about what the preview status changes mean here.

@TheMrMilchmann
Copy link

Can it be NULL while it's in preview perhaps?

Preview features are incorporated into the JLS & JVMS. All spec-compliant implementations must implement support for preview features. Thus, this function should always exist.

So is there some kind of change with what can be relied on with the behaviour or IsVirtualThread between versions?

Indeed, the major difference between "stable" and preview features is that the behavior (or the even existence of the API going forward) is not guaranteed. Albeit unlikely, it is possible that a preview function is added but its signature is changed or it is removed in the next release. This is probably something that needs to be considered in the context of #25.

Maybe IsVirtualThread should only be exposed once it's out of preview perhaps?

Ideally no, because the point of preview features is that people can test them and give feedback before the feature is stabilized. As this project is just a low-level wrapper for JNI, I think we should expose preview functions just like everything else.

For further information on preview features, see

@rib
Copy link
Contributor

rib commented Jun 28, 2023

Albeit unlikely, it is possible that a preview function is added but its signature is changed or it is removed in the next release

Ooof, yeah that really breaks assumptions with the current way we just have one large vtable that we assume we can safely just append to - we wouldn't be able to handle a situation where an API ended up with a different signature or got removed unless we moved to a union as suggested in #25

Since I was recently scrutinizing the macros in the jni-rs crate I actually realized the jni-rs crate currently incorrectly dereferences v1.2 functions without knowing that the version is >= 1.2 which did generally convince me that it would be a lot better for this crate to expose the function pointers via union instead of one single table.

It is very easy atm to unwittingly use an API that might not be valid for the current known JNI version.

Although I hope there's no example already of an API changing between versions a union would allow for that kind of change.

The main issue probably is just the amount of busy work / boilerplate involved in introducing a union with separate tables for each version. It should probably be done via some kind of build.rs codegen or macros.

Maybe IsVirtualThread should only be exposed once it's out of preview perhaps?

Ideally no, because the point of preview features is that people can test them and give feedback before the feature is stabilized. As this project is just a low-level wrapper for JNI, I think we should expose preview functions just like everything else.

yeah, makes sense.

@rib rib added this to the Release 0.4 milestone Jul 25, 2023
@rib rib merged commit 8379800 into jni-rs:master Aug 31, 2023
@rib
Copy link
Contributor

rib commented Aug 31, 2023

For reference, I'll look at adding IsVirtualThread as a separate follow up.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants